Skip to content

Enable ctas with unmanaged hive table #15990

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Enable ctas with unmanaged hive table #15990

wants to merge 2 commits into from

Conversation

@rguillome
Copy link

@rguillome rguillome commented Apr 23, 2021 •
edited
Loading

Test plan - TU to create an table as a select with an external location with presto-hive plugin and a manual TI to test the creation under EMR, using a Glue catalog and seeing the results in S3 and Athena

This PR allow the creation of an unmanaged table (with external location specified) as a select result (CTAS) with Hive Plugin

Co-authored-by: alexjo2144 [email protected]

Fix:
#15946

Based on
trino PR #2669
trino PR #3755

== RELEASE NOTES ==

Hive Changes
* Add ability to create table with an external location when the catalog configuration 
  ``hive.non-managed-table-creates-enabled`` and ``hive.non-managed-table-writes-enabled`` set to ``true``

@v-jizhang v-jizhang self-requested a review April 23, 2021 15:15
@v-jizhang
Copy link
Contributor

v-jizhang commented Apr 23, 2021

@rguillome This is cherry pick of trinodb/trino#3755 and trinodb/trino#2669. Please add co-author. Thanks

v-jizhang
v-jizhang previously approved these changes May 17, 2021
View reviewed changes
Copy link
Contributor

@v-jizhang v-jizhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rguillome
Copy link
Author

rguillome commented May 31, 2021

Hi @v-jizhang

This PR is still waiting approval and I just saw another PR with the same purpose (#16147).
Is this one won't be merged anymore ?

@rguillome rguillome force-pushed the enable-ctas-with-unmanaged-hive-table branch 2 times, most recently from da9508b to 6aa52d9 Compare May 31, 2021 14:24
@ashishtadose
Copy link
Contributor

ashishtadose commented Jun 7, 2021

Hi @v-jizhang

This PR is still waiting approval and I just saw another PR with the same purpose (#16147).
Is this one won't be merged anymore ?

Hi @rguillome, we happened to address this recently in our distro so I raised a PR not knowing there's already some WIP on this. I will close the other PR.

@rguillome
Copy link
Author

rguillome commented Jun 9, 2021 •
edited
Loading

Thanks @ashishtadose !
I hoped one of them would be approved in few days no matter what the contributor is. I have a tool release waiting for this since some weeks.

@ajaygeorge
Copy link
Contributor

ajaygeorge commented Jun 9, 2021

@rguillome Please squash the 6 commits in the PR (some of them are merge commits)
Also refer to Release Notes wiki : https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines
The current PR release notes does not match what we need.

@rguillome rguillome force-pushed the enable-ctas-with-unmanaged-hive-table branch from 6aa52d9 to 644df66 Compare June 10, 2021 14:03
@rguillome
Copy link
Author

rguillome commented Jun 10, 2021 •
edited
Loading

Hi @ajaygeorge
I made all the changes: Hope this is ok now 😏

@rguillome rguillome force-pushed the enable-ctas-with-unmanaged-hive-table branch 2 times, most recently from dcde93f to 5c4b803 Compare June 13, 2021 13:15
@rguillome
Copy link
Author

rguillome commented Jun 16, 2021

Hi @ajaygeorge,

Please let me know what's why Facebook integration is failing (I thought I have seen succeed a day before...) and what's missing/wrong about this PR.

@ajaygeorge
Copy link
Contributor

ajaygeorge commented Jun 16, 2021

@rguillome can you try triggering the build again by doing an git amend and push with no changes.
I think there were some permission issues which was causing errors.

@rguillome rguillome force-pushed the enable-ctas-with-unmanaged-hive-table branch from 5c4b803 to a08d4ba Compare June 17, 2021 07:24
@rguillome
Copy link
Author

rguillome commented Jun 21, 2021

Hi @ajaygeorge ,

I made the push force 5 days ago with no more luck.

Regards,

@ajaygeorge
Copy link
Contributor

ajaygeorge commented Jun 21, 2021

@cocozianu can you please take a look at why the facebook integration build is not working. Looks like a new build is not getting fired for this PR.

@rguillome
Copy link
Author

rguillome commented Jul 6, 2021

Hi @ajaygeorge and @cocozianu

There is still a facebook integration error on this PR. Please let me know if I can change something to help going through this step.

Regards,

@cocozianu
Copy link
Contributor

cocozianu commented Jul 6, 2021

Hi @rguillome it's cleared now, sorry for the inconvenience.

@rguillome
Copy link
Author

rguillome commented Jul 16, 2021

Hi @cocozianu : what's the next step for this PR, how can it be handled to be merged ?

Regards,

@rguillome
Copy link
Author

rguillome commented Aug 2, 2021

Hi @cocozianu : what's the next step for this PR, how can it be handled to be merged ?

Regards,

Please up !

@rguillome
Copy link
Author

rguillome commented Aug 19, 2021

Hi @ajaygeorge and @cocozianu

Is there any chance this PR would be integrated soon ?

@rohanpednekar
Copy link
Contributor

rohanpednekar commented Oct 4, 2021

@aweisberg Do you think we can merge for the next release if it looks good to you as well?

@yingsu00 yingsu00 self-requested a review November 3, 2021 01:56
@rguillome
Copy link
Author

rguillome commented Jan 7, 2022

Hi there,

Happy new year to Presto's team 🎉

Is there a chance that PR would be merge ? Time is passing and I would like to avoid maintaining a fork with work to integrate incoming changes in my current production version of PrestoSQL

Please let me know what do you plan for it

Regards

@aweisberg
Copy link
Contributor

aweisberg commented Jan 7, 2022

This doesn't bring in all of the tests from Trino so maybe we should go with #16147?

@aweisberg aweisberg mentioned this pull request Jan 7, 2022
Open
@rguillome
Copy link
Author

rguillome commented Jan 10, 2022 •
edited
Loading

This doesn't bring in all of the tests from Trino so maybe we should go with #16147?

As mentionned in the PR first comment, it was deliberate since a CTAS is a table creation from query results and not writing to an existing table but perhaps it's better to stick to trino's view.

I will close this one and go to the #16147

@rguillome rguillome force-pushed the enable-ctas-with-unmanaged-hive-table branch 9 times, most recently from 1026084 to 8de0e9c Compare February 15, 2022 12:13
@rguillome
Copy link
Author

rguillome commented Feb 15, 2022

This doesn't bring in all of the tests from Trino so maybe we should go with #16147?

As mentionned in the PR first comment, it was deliberate since a CTAS is a table creation from query results and not writing to an existing table but perhaps it's better to stick to trino's view.

I will close this one and go to the #16147

Hi @aweisberg,

As the other PR #16147 introducing CTAS has not changed since your last comment, I managed to make this one compliant with the missings tests (there is one test more than the #16147 in the product-test module).

Rebase to the current master has been made too.
Please let me know if there is anything missing yet.

@rguillome rguillome force-pushed the enable-ctas-with-unmanaged-hive-table branch from 8de0e9c to b80eeec Compare February 17, 2022 14:16
@rohanpednekar
Copy link
Contributor

rohanpednekar commented Feb 17, 2022

Hey @yingsu00, could you please help with the final revive and get this merged? Thanks!

@rguillome
Copy link
Author

rguillome commented Mar 22, 2022

Hi, any news from this one please ?

@prestoprobot
Copy link

prestoprobot commented Feb 20, 2023

@yingsu00 Friendly ping. Please review this PR. Thank you!

@rguillome
Copy link
Author

rguillome commented May 11, 2023

Hi,
it's been a long time now. Let me know if there is a chance that it would be merged...

@wanglinsong wanglinsong requested a review from a team as a code owner July 6, 2024 04:32
@wanglinsong wanglinsong requested a review from presto-oss July 6, 2024 04:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@v-jizhang v-jizhang v-jizhang left review comments

@yingsu00 yingsu00 Awaiting requested review from yingsu00

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

awaiting-community-help

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@rguillome @v-jizhang @ashishtadose @ajaygeorge @cocozianu @rohanpednekar @aweisberg @prestoprobot